Fixes in power_rev #55
Conversation
|
Included the Manifest.toml to use 1.0-dev branch in IA |
|
Once JuliaIntervals/IntervalArithmetic.jl#533 is merged (in 1.0-dev branch), all should pass. |
|
The remaining broken tests are some edge cases... |
|
with this branch some broken tests are not broken anymore, but it also introduces some new failures, both using 1.0-dev and JuliaIntervals/IntervalArithmetic.jl#533 🤔. Is 533 rebased to 1.0-dev? |
|
I think JuliaIntervals/IntervalArithmetic.jl#533 branch includes the changes in .lo and .hi (JuliaIntervals/IntervalArithmetic.jl#531), so I would say yes, it includes changes already in 1.0-dev. I agree, it seems that it creates some new failures. I think those failures are related to the properties of the implementation that I'm trying to clarify. I don't think those aspects are related to the implementation in IA, but in the way that |
It should use the last commit in the 1.0-dev branch
|
hups, I had a small misunderstanding in JuliaIntervals/IntervalArithmetic.jl#533 . In my head I was only thinking about I'll take another look at this before / after 533 now that I know what I'm talking about :) |
| @@ -19,7 +19,7 @@ function plus_rev(a::Interval, b::Interval, c::Interval) # a = b + c | |||
| return a, b_new, c_new | |||
| end | |||
|
|
|||
| plus_rev(a,b,c) = plus_rev(promote(a,b,c)...) | |||
| plus_rev(a::F, b::F, c) where {T, F<:Interval{T}} = plus_rev(a, b, F(c)) | |||
There was a problem hiding this comment.
shouldn't this (and the others) be
function plus_rev(a::Interval{T}, b::Interval{S}, c::Interval(R})
TSR = promote_type(T, S, R) # not sure if you can do it in one line, but you get the idea
F = Interval{TSR}
return plus_rev(F(a), F(b), F(c))
endbut it gets pretty tedious... I get the idea of not having promotion Float64 --> Interval{Float64}, but I think a promotion rule for Interval{T}, Interval{S} would be harmless.
In both cases something like plus_rev(1.0, 1..2, 3..4) wouldn't work. This can be a bug (if you think in the more ducktyping way) or a feature (if you come from a strong static typing programming language)
There was a problem hiding this comment.
Regarding the second method, which previously had a promotion, I truly don't recall what was the motivation. I guess it was a way of avoiding using promote...
In any case, let me note that many operations in the 1.0-dev branch of IA impose the same Interval{T} for the operands: e.g., (1.0f0 .. 2.f0) - ( 1.5 .. 2.5) throws a MethodError due to ambiguities...
There was a problem hiding this comment.
if it's because of ambiguity, then it's a bug (or at least misleading error message). In general, it's not necessarily wrong to disallow implicit conversions, strongly typed languages do it and for good reasons (see this accident ), but it doesn't feel julian, also for every function with multiple arguments you would need to manually 1) either define methods for all possible missing cases 2) or have an own interval _promote_interval to have things like f(1.0, 1..2, 2.0) work.
There was a problem hiding this comment.
but this is probably tangential to this PR
Sorry, my mistake... I don't remember what was the motivation to make those changes, apart from having a similar coding style than in 1.0-dev... Should I revert those changes, and only concentrate in the |
|
no need to revert, it's a good PR and it does fix a few tests (don't know why everything is failing, will investigate soon). Apart for my one comment (which can also be delegated to further PRs) LGTM |
|
Sorry for the very late response... What's the status here? |
No description provided.